Skip to content

Ensure exploration_results is a list #607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 15, 2025

Conversation

r3kste
Copy link
Contributor

@r3kste r3kste commented Jul 3, 2025

This PR ensures that exploration_results in MultistartInfo is a list.

Summary

  1. Introduce dataclass _InternalExplorationResult, which is returned by run_explorations function.
  2. Test that exploration values is of type list[float].

Possible Changes

I would like to get feedback on whether the following changes should be included.

  • Use a less strict check for dtype of exploration_results. For example, a subdtype of np.floating
  • Add a check to assert that exploration_results is positive for minimize and negative for maximize.

Copy link

codecov bot commented Jul 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/optimagic/optimization/multistart.py 91.19% <100.00%> (+0.22%) ⬆️
src/optimagic/optimization/process_results.py 94.11% <ø> (ø)
src/optimagic/visualization/history_plots.py 90.84% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@timmens timmens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thank you!

Regarding your questions:

  1. In optimagic we want to stick with float64 for now. If we ever want to be more lenient, then we do it for the whole codebase at once.
  2. Such a check is not necessary imo.

@timmens timmens requested a review from janosg July 7, 2025 12:22
@timmens
Copy link
Member

timmens commented Jul 7, 2025

Hey @r3kste!

I discussed the problem / inconsistency with Janos. We realized that instead of adhering to the output of run_explorations, we'd rather change the output type! That is, we believe it is more fitting and in line with the rest of the codebase if run_explorations returns a dictionary of list[float]'s.

Can you rename the PR and make the required changes for this?

@r3kste
Copy link
Contributor Author

r3kste commented Jul 7, 2025

Thanks for the quick feedback.

That is, we believe it is more fitting and in line with the rest of the codebase if run_explorations returns a dictionary of list[float]'s.

The "sorted_sample" key in the output dictionary of run_explorations is currently a 2D numpy array. So I don't think we can change the return type to dictionary of list[float].

If we are going ahead with "sorted_values" being a list[float], We could change return type of run_explorations to dict[str, Any] and keep "sorted_sample" as a NDArray.

@janosg
Copy link
Member

janosg commented Jul 8, 2025

The exploration result has fixed fields, so it should not be a dict at all but a dataclass (say InternalExplorationResult) so we get type checking for attribute access.

The reason why I would have preferred a list is that this is in principle a history and for histories we usually use lists of floats or lists of arrays as this is the most intuitive way to store histories. However, I don't have a strong opinion here. So you can choose if InternalExplorationResult has list or array attributes but we definitely don't want to use a dict here.

@r3kste r3kste changed the title Ensure exploration_results is a numpy array Ensure exploration_results is a list Jul 10, 2025
@r3kste r3kste force-pushed the exploration_results_patch branch from 54e1064 to b87db0d Compare July 14, 2025 08:22
@timmens timmens merged commit 4e58df8 into optimagic-dev:main Jul 15, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants